Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves and fixes the web-client UI #668

Merged
merged 38 commits into from
May 13, 2024
Merged

Conversation

JulienVig
Copy link
Collaborator

@JulienVig JulienVig commented May 3, 2024

Addresses parts of #627 #628, #662, #665 and various things noted while working on the LUS COVID demo

  • Rework website navigation to make it more intuitive. Home page now links to tutorial information, existing tasks and custom task creation. The sidebar links to the home page, existing tasks, task creation, model evaluation, model library and the tutorial information. Settings has been merged with model library (since it only had the model library toggle button)
  • Add error handling when selected data (e.g. wrong CSV header, image file not found in CSV labels etc)
  • Removed pre-defined tasks geotags and skin mnist because no data is provided and training has not been not tested
  • Harmonize the outdated tutorial information UI with the rest of the website.
  • Remove l18n internationalization plugin, only english is currently used and there are no plans to add new languages.
  • Rework ModelCaching, current behavior now explicit and card moved to training page
  • Improve the training charts with loss plots, tooltip, and dynamic y axis range
  • Add an explanation on what pre-defined "DISCOllaboratives" are

Addresses #627
Addresses #628
Addresses #662
Addresses #665

@JulienVig JulienVig self-assigned this May 3, 2024
@JulienVig JulienVig marked this pull request as draft May 3, 2024 14:32
@JulienVig JulienVig changed the title 662 improve tasks julien Improves and fixes the pre-defined task UI May 3, 2024
@JulienVig JulienVig changed the title Improves and fixes the pre-defined task UI Improves and fixes the web-client UI May 7, 2024
@JulienVig JulienVig marked this pull request as ready for review May 7, 2024 15:58
@JulienVig JulienVig requested a review from tharvik May 7, 2024 15:58
Copy link
Collaborator

@tharvik tharvik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, a fresher and clearer interface, great work! ✨

as usual, nothing blocking, only a few comments

  • good idea to remove i18n, it's was barely used anyway
  • and rm useles tests on Landing is a nice too
  • for the geotags & skin_mnist task, nothing to salvage?

@@ -17,7 +17,8 @@ export const cifar10: TaskProvider = {
tradeoffs: 'Training success strongly depends on label distribution',
dataFormatInformation: 'Images should be of .png format and of size 32x32. <br> The label file should be .csv, where each row contains a file_name, class. <br> <br> e.g. if you have images: 0.png (of a frog) and 1.png (of a car) <br> labels.csv contains: (Note that no header is needed)<br> 0.png, frog <br> 1.png, car',
dataExampleText: 'Below you can find 10 random examples from each of the 10 classes in the dataset.',
dataExampleImage: 'https://storage.googleapis.com/deai-313515.appspot.com/example_training_data/cifar10-example.png'
dataExampleImage: 'https://storage.googleapis.com/deai-313515.appspot.com/example_training_data/cifar10-example.png',
sampleDatasetLink: 'https://www.kaggle.com/competitions/cifar-10/data'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding that to datasets/populate instead of pulling from our own archive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work when the webapp is deployed? I think datasets/populate can only be used in local isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, do keep it in cifar10's Task and add it to datasets/populate so that we ensure that we also train our tests with the sample dataset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so some cifar10 data is already included in the example_training_data (which requires manipulation to used in the webapp...) but I wanted to provide a public facing link too.

discojs/discojs-node/src/data/image_loader.spec.ts Outdated Show resolved Hide resolved
cli/src/data.ts Outdated Show resolved Hide resolved
server/tests/e2e/federated.spec.ts Outdated Show resolved Hide resolved
web-client/src/components/training/Trainer.vue Outdated Show resolved Hide resolved
web-client/src/components/training/Trainer.vue Outdated Show resolved Hide resolved
web-client/src/components/training/TrainingInformation.vue Outdated Show resolved Hide resolved
web-client/src/components/training/TrainingInformation.vue Outdated Show resolved Hide resolved
web-client/src/components/training/TrainingInformation.vue Outdated Show resolved Hide resolved
@JulienVig
Copy link
Collaborator Author

JulienVig commented May 8, 2024

for the geotags & skin_mnist task, nothing to salvage?

There is a map modal implemented in Tester.vue for the geotag task which can be interesting but it relies on https://disco-polygon.web.app/ which doesn't exist anymore. I couldn't find what was the training data and with the base map missing I'm going to remove the task. It would be interesting to keep it in mind and go back to it if we ever want to implement another location based task
The skin mnist is very similar to LUS Covid but multi-class classification. I don't think it's worth uploading a new sample dataset and some work is required to create a correct CSV file to use the kaggle dataset in Disco

@JulienVig
Copy link
Collaborator Author

@tharvik It looks like the npm deployment broke the build-server-docker action?

@tharvik
Copy link
Collaborator

tharvik commented May 8, 2024

@tharvik It looks like the npm deployment broke the build-server-docker action?

looks like upstream released a broken nodejs version, followup there. openned a PR fixing that #672

@JulienVig JulienVig merged commit d454f30 into develop May 13, 2024
23 checks passed
@JulienVig JulienVig deleted the 662-improve-tasks-julien branch May 13, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants